Remove some flags that are not enabled by default#3772
Open
Remove some flags that are not enabled by default#3772
Conversation
zatteo
requested review from
JF-Cozy,
doubleface,
lethemanh and
rezk2ll
as code owners
WalkthroughThis PR removes runtime feature-flag checks and related imports for multiple Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Feature by default now
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/views/Modal/QualifyFileView.jsx (1)
31-38:⚠️ Potential issue | 🔴 CriticalSpread
themesListto flatten the theme options array.At line 37,
themesListmust be spread with...to avoid creating a nested structure. Without spreading,themesWithNonebecomes[noneTheme, themesList_array], and when the code iterates at line 41 and tries to accesstheme.items.map()at line 49, it will fail because the second element is an array without anitemsproperty.Proposed fix
const themesWithNone = [ { id: 'none', items: [], label: t('Scan.none') }, - themesList + ...themesList ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/views/Modal/QualifyFileView.jsx` around lines 31 - 38, The themesWithNone array currently nests themesList as its second element causing downstream code (e.g., iteration that accesses theme.items) to fail; change construction of themesWithNone (where themesWithNone and themesList are defined) to flatten themesList by using the spread operator so the array becomes [noneTheme, ...themesList] instead of [noneTheme, themesList], ensuring each element has an items property when iterating over themesWithNone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/modules/views/Modal/QualifyFileView.jsx`:
- Around line 31-38: The themesWithNone array currently nests themesList as its
second element causing downstream code (e.g., iteration that accesses
theme.items) to fail; change construction of themesWithNone (where
themesWithNone and themesList are defined) to flatten themesList by using the
spread operator so the array becomes [noneTheme, ...themesList] instead of
[noneTheme, themesList], ensuring each element has an items property when
iterating over themesWithNone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0adc57d-5c00-423a-afb7-31b98966ceb0
📒 Files selected for processing (18)
jestHelpers/setup.jssrc/hooks/useFolderSort/index.spec.jsxsrc/hooks/useFolderSort/index.tssrc/hooks/useKeyboardShortcuts.spec.jsxsrc/hooks/useKeyboardShortcuts.tsxsrc/hooks/useOnLongPress/helpers.jssrc/hooks/useUpdateFavicon/index.tsxsrc/lib/flags.jssrc/modules/actions/components/personalizeFolder.jssrc/modules/drive/Toolbar/personalizeFolder/PersonalizeFolderItem.jsxsrc/modules/filelist/icons/FileIconMime.jsxsrc/modules/layout/Layout.jsxsrc/modules/public/PublicLayout.jsxsrc/modules/upload/NewItemHighlightProvider.jsxsrc/modules/views/Folder/virtualized/useScrollToHighlightedItem.jsxsrc/modules/views/Modal/QualifyFileView.jsxsrc/modules/views/Public/PublicFolderView.jsxtest/components/AppLike.jsx
💤 Files with no reviewable changes (9)
- src/lib/flags.js
- src/modules/filelist/icons/FileIconMime.jsx
- src/hooks/useKeyboardShortcuts.spec.jsx
- src/modules/drive/Toolbar/personalizeFolder/PersonalizeFolderItem.jsx
- src/hooks/useFolderSort/index.spec.jsx
- src/modules/actions/components/personalizeFolder.js
- src/hooks/useKeyboardShortcuts.tsx
- src/modules/upload/NewItemHighlightProvider.jsx
- src/hooks/useOnLongPress/helpers.js
rezk2ll
approved these changes
Mar 30, 2026
JF-Cozy
approved these changes
Mar 30, 2026
Contributor
JF-Cozy
left a comment
JF-Cozy
left a comment
There was a problem hiding this comment.
don't forget to remove them from Notion after merging :)
doubleface
reviewed
Mar 30, 2026
Contributor
doubleface
left a comment
doubleface
left a comment
There was a problem hiding this comment.
Looks like there are still some unit tests to fix
Feature by default now
Feature by default now
Feature by default now
Feature by default now
Feature by default now
Feature by default now
zatteo
force-pushed
the
feat/remove-some-flags
branch
from
c490cd0 to
dfa6900
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
(11 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| NewItemHighlightProvider.jsx | 9.69 → 10.00 | Complex Method |
| useScrollToHighlightedItem.jsx | 9.55 → 9.58 | Complex Method |
| useKeyboardShortcuts.spec.jsx | 7.92 → 7.95 | Complex Method |
| useKeyboardShortcuts.tsx | 8.56 → 8.57 | Complex Method |
| PublicFolderView.jsx | 8.57 → 8.59 | Complex Method |
| index.ts | 9.26 → 9.58 | Complex Method, Complex Conditional |
| index.spec.jsx | 9.10 → 9.39 | Code Duplication |
| index.tsx | 9.69 → 10.00 | Complex Conditional |
| PersonalizeFolderItem.jsx | 9.54 → 9.69 | Complex Conditional |
| FileIconMime.jsx | 9.24 → 9.39 | Complex Method, Complex Conditional |
| helpers.js | 9.69 → 10.00 | Complex Method |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Member
Author
|
I will check that all these flags are already enabled in every environments before merging. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useOnLongPress/helpers.spec.jsx (2)
77-99:⚠️ Potential issue | 🟠 MajorMissing test coverage for single-click behavior.
The
setSelectedItemsmock is added (line 95) but no test verifies it's called. Perhelpers.jslines 26-31, single-click triggerssetSelectedItems({ [file._id]: file }). This is a primary code path that should be covered.Additionally,
toggle: mockToggleat line 92 is dead code—handleClickno longer uses atoggleparameter.🧪 Proposed fix: Add single-click test and remove unused toggle
params: { event, disabled, isRenaming, file, openLink: mockOpenLink, - toggle: mockToggle, lastClickTime, setLastClickTime: jest.fn(), setSelectedItems: jest.fn(), onInteractWithFile: jest.fn() } } }Add a test for single-click behavior:
describe('for single click', () => { beforeEach(() => { MockDate.set('2025-01-01T12:00:01.000Z') // 1 second after first click (not a double-click) }) it('should call setSelectedItems with file', () => { const file = { _id: 'test-file-id' } const { params } = setup({ file }) handleClick(params) expect(params.setSelectedItems).toHaveBeenCalledWith({ [file._id]: file }) expect(mockOpenLink).not.toHaveBeenCalled() }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useOnLongPress/helpers.spec.jsx` around lines 77 - 99, Add a unit test covering the single-click path of handleClick: in the spec use setup(...) to create params (with a file like { _id: 'test-file-id' }), set MockDate to one second after the prior click so it is not treated as a double-click, call handleClick(params), and assert params.setSelectedItems was called with { [file._id]: file } and that mockOpenLink was not called; also remove the unused mockToggle from the setup params and any references since handleClick no longer consumes a toggle parameter.
138-138:⚠️ Potential issue | 🟡 MinorTypo in test description.
"renainming" should be "renaming".
✏️ Proposed fix
- it('it should do nothing when renainming', () => { + it('it should do nothing when renaming', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useOnLongPress/helpers.spec.jsx` at line 138, Update the test case description that currently reads "it should do nothing when renainming" to the correct spelling "it should do nothing when renaming" in the it(...) call in the useOnLongPress/helpers.spec.jsx test (the it block with the misspelled string "it should do nothing when renainming").
🧹 Nitpick comments (2)
src/hooks/useOnLongPress/helpers.spec.jsx (1)
106-113: Remove or update the skipped test.This
xittest referencesmockToggle, which is no longer used byhandleClick. Either delete this test or update it to verifysetSelectedItemsbehavior per the new implementation.♻️ Proposed fix
- // should create a real life test to replace toggle by final func - xit('should only toggle by default', () => { - const { params } = setup({}) - handleClick(params) - - expect(mockToggle).toHaveBeenCalledWith(ev) - expect(mockOpenLink).not.toHaveBeenCalled() - })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useOnLongPress/helpers.spec.jsx` around lines 106 - 113, Update the skipped test 'should only toggle by default' in useOnLongPress/helpers.spec.jsx: remove references to mockToggle and instead assert the new behavior by calling handleClick(params) (use setup to get params/ev) and verifying setSelectedItems is called with the expected selection payload and that mockOpenLink is not called; alternatively delete the entire xit block if this case is no longer applicable. Ensure you reference handleClick, setup, setSelectedItems, mockOpenLink and ev so the assertion matches the current implementation.src/modules/views/Folder/virtualized/useScrollToHighlightedItem.jsx (1)
46-48: Pre-existing dead code:targetIndex === -1check is unreachable.Since Line 38 already guards with
!indexById.has(targetItem._id), theMap.get()on Line 43 will always return a valid non-negative index. ThetargetIndex === -1condition can never be true.Consider simplifying in a follow-up:
♻️ Optional simplification
const targetIndex = indexById.get(targetItem._id) const virtuosoHandle = virtuosoRef?.current - if (targetIndex === -1 || !virtuosoHandle) { + if (!virtuosoHandle) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/views/Folder/virtualized/useScrollToHighlightedItem.jsx` around lines 46 - 48, The check for targetIndex === -1 in the useScrollToHighlightedItem flow is dead code because indexById.has(targetItem._id) already guarantees Map.get() yields a valid index; remove the unreachable condition and simplify the early return to only test virtuosoHandle (i.e., replace "if (targetIndex === -1 || !virtuosoHandle) { return }" with "if (!virtuosoHandle) { return }"), keeping references to targetIndex, virtuosoHandle, indexById, and targetItem._id so intent and readability remain clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/hooks/useOnLongPress/helpers.spec.jsx`:
- Around line 77-99: Add a unit test covering the single-click path of
handleClick: in the spec use setup(...) to create params (with a file like {
_id: 'test-file-id' }), set MockDate to one second after the prior click so it
is not treated as a double-click, call handleClick(params), and assert
params.setSelectedItems was called with { [file._id]: file } and that
mockOpenLink was not called; also remove the unused mockToggle from the setup
params and any references since handleClick no longer consumes a toggle
parameter.
- Line 138: Update the test case description that currently reads "it should do
nothing when renainming" to the correct spelling "it should do nothing when
renaming" in the it(...) call in the useOnLongPress/helpers.spec.jsx test (the
it block with the misspelled string "it should do nothing when renainming").
---
Nitpick comments:
In `@src/hooks/useOnLongPress/helpers.spec.jsx`:
- Around line 106-113: Update the skipped test 'should only toggle by default'
in useOnLongPress/helpers.spec.jsx: remove references to mockToggle and instead
assert the new behavior by calling handleClick(params) (use setup to get
params/ev) and verifying setSelectedItems is called with the expected selection
payload and that mockOpenLink is not called; alternatively delete the entire xit
block if this case is no longer applicable. Ensure you reference handleClick,
setup, setSelectedItems, mockOpenLink and ev so the assertion matches the
current implementation.
In `@src/modules/views/Folder/virtualized/useScrollToHighlightedItem.jsx`:
- Around line 46-48: The check for targetIndex === -1 in the
useScrollToHighlightedItem flow is dead code because
indexById.has(targetItem._id) already guarantees Map.get() yields a valid index;
remove the unreachable condition and simplify the early return to only test
virtuosoHandle (i.e., replace "if (targetIndex === -1 || !virtuosoHandle) {
return }" with "if (!virtuosoHandle) { return }"), keeping references to
targetIndex, virtuosoHandle, indexById, and targetItem._id so intent and
readability remain clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e973f17-c71c-4854-bec7-bd611a117257
📒 Files selected for processing (21)
jestHelpers/setup.jssrc/hooks/useFolderSort/index.spec.jsxsrc/hooks/useFolderSort/index.tssrc/hooks/useKeyboardShortcuts.spec.jsxsrc/hooks/useKeyboardShortcuts.tsxsrc/hooks/useOnLongPress/helpers.jssrc/hooks/useOnLongPress/helpers.spec.jsxsrc/hooks/useUpdateFavicon/index.spec.jsxsrc/hooks/useUpdateFavicon/index.tsxsrc/lib/flags.jssrc/modules/actions/components/personalizeFolder.jssrc/modules/drive/Toolbar/personalizeFolder/PersonalizeFolderItem.jsxsrc/modules/filelist/icons/FileIconMime.jsxsrc/modules/layout/Layout.jsxsrc/modules/public/PublicLayout.jsxsrc/modules/upload/NewItemHighlightProvider.jsxsrc/modules/views/Folder/virtualized/useScrollToHighlightedItem.jsxsrc/modules/views/Folder/virtualized/useScrollToHighlightedItem.spec.jsxsrc/modules/views/Modal/QualifyFileView.jsxsrc/modules/views/Public/PublicFolderView.jsxtest/components/AppLike.jsx
💤 Files with no reviewable changes (10)
- src/lib/flags.js
- src/modules/views/Folder/virtualized/useScrollToHighlightedItem.spec.jsx
- src/modules/actions/components/personalizeFolder.js
- src/modules/filelist/icons/FileIconMime.jsx
- src/hooks/useKeyboardShortcuts.tsx
- src/hooks/useFolderSort/index.spec.jsx
- src/modules/drive/Toolbar/personalizeFolder/PersonalizeFolderItem.jsx
- src/hooks/useUpdateFavicon/index.spec.jsx
- src/modules/upload/NewItemHighlightProvider.jsx
- src/hooks/useOnLongPress/helpers.js
✅ Files skipped from review due to trivial changes (5)
- src/modules/views/Modal/QualifyFileView.jsx
- src/hooks/useFolderSort/index.ts
- src/modules/public/PublicLayout.jsx
- src/modules/layout/Layout.jsx
- src/modules/views/Public/PublicFolderView.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/hooks/useUpdateFavicon/index.tsx
- test/components/AppLike.jsx
Contributor
|
@zatteo Please, move forward with this PR. It's complicated to have Drive locally without the behavior we have in production because of the flags. So yes, let's have this PR merged in some way :) |
Member
Author
It's in progress. We found a production environment where we did not enabled these flags so we are trying to enable them smoothly before removing them totally in the code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests